"Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> @@ -576,6 +579,17 @@ int cmd_add(int argc,
>  		string_list_clear(&only_match_skip_worktree, 0);
>  	}
>  
> +	if (!show_only && !no_verify) {
> +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +
> +		strvec_pushf(&opt.env, "GIT_INDEX_FILE=%s",
> +			     repo_get_index_file(repo));
> +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> +			exit_status = 1;
> +			goto finish;
> +		}
> +	}
> +
>  	transaction = odb_transaction_begin(repo->objects);
>  
>  	ps_matched = xcalloc(pathspec.nr, 1);

Hmph, unless I am confused, I am a bit disappointed.  The code
snippet whose beginning we can see in the post context is
preparation for determining which paths are going to be updated, and
this new code happens before anything is added to the in-core index.

The hook takes no clue from anything derived from the command line,
not even the pathspec (or list of individual paths computed using
the pathspec by the command) or the mode of operation like '-u' or
'--renormalize'.  I am not sure how effective a decision the invoked
hook can make to approve or deny in this lack of information.

Also I am not sure what good it is doing to pass GIT_INDEX_FILE as
an environment variable.  If this were a hook that is invoked by
"git commit", which may be doing a partial commit "git commit [-o]
path", the command involves multiple on-disk index files to allow
the changes to named paths jump over already added changes to other
paths, but "git add path" is always inclusive of already added
changes, and does not use anything but the main index file being
used.

So,...

Junio C Hamano <gitster@pobox.com> writes:

> The hook takes no clue from anything derived from the command line,
> not even the pathspec (or list of individual paths computed using
> the pathspec by the command) or the mode of operation like '-u' or
> '--renormalize'.  I am not sure how effective a decision the invoked
> hook can make to approve or deny in this lack of information.

And I do not necessarily suggest passing the pathspec arguments or
command line options that the "git add" command received from its
caller down to the hook, which will force hook authors to emulate
what "git add" would do to these arguments and options, and they
will certainly get it wrong.

I wonder if we can split write_locked_index() into two so that
writing out the in-core index to the temporary/lockfile can happen
separately from the call to commit_locked_index().  If we can do so,
then the following would become a viable and better implementation
of this new feature to run the "pre-add" hook:

 * Determine if we will need to run this "pre-add" hook, at the
   location in the code you addded the run_hooks_opt() invocation,
   but do *NOT* run any hook there yet.

 * Instead, create a temporary copy of the index file if the above
   says "Yes, we are going to run the hook".

 * Let the code path to update the in-core index, i.e., letting
   everythning up to the "finish:" label to run normally.

 * Perform the first-half of the write_locked_index(), writing the
   new index contents into the lockfile, but stopping before
   committing it to the final name.

 * If we are running the hook, run it with two arguments, the name
   of the temporary copy of the original index we created earlier,
   and the name of this lockfile that has the proposed contents of
   the index if the hook allowed "git add" to proceed.

 * If we ran the hook and hook succeeded, or if we did not have to
   run the hook at all, then commit the lockfile.  Otherwise abort
   the "git add" command and rollback_lock_file().

 * Remove the temporary file we created earlier (if any).

Your hooks can "GIT_INDEX_FILE=$1 git diff --cached --name-only" to
find out which paths already had changes added before this
invocation of "git add", and similarly using $2 get the list of
paths that will add further changes with this invocation.  The
latter set of paths you can inspect to see if you like the
additional changes brought in, perhaps like

    #!/bin/sh
    paths=$(GIT_INDEX_FILE=$2 git diff --cached --name-only)
    GIT_INDEX_FILE=$1 git diff $paths >patch.txt

    if grep "^+.*secret" patch.txt
    then
        echo "do not divulge company secret!" >&2
	exit 1
    fi

or something.

Thank you for the thorough feedback -- v2 follows the architecture you outlined.

The hook now runs after staging is computed, receiving two positional arguments: a temporary copy of the original index ($1) and the lockfile containing the proposed index ($2). Hook authors inspect the computed result directly.

One trade-off: since staging must complete to produce $2, blobs are written to the object store before the hook fires. I think it's worth it though for the reasons you mentioned re: hook authors. 

The CI failure is a result of an ar/parallel-hooks conflict. 

Appreciate the time, effort, and thought you put into review and feedback. Excited to hear back

Chandra Kethi-Reddy

Sent with Proton Mail secure email.

On Wednesday, February 11th, 2026 at 12:31 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The hook takes no clue from anything derived from the command line,
> > not even the pathspec (or list of individual paths computed using
> > the pathspec by the command) or the mode of operation like '-u' or
> > '--renormalize'.  I am not sure how effective a decision the invoked
> > hook can make to approve or deny in this lack of information.
> 
> And I do not necessarily suggest passing the pathspec arguments or
> command line options that the "git add" command received from its
> caller down to the hook, which will force hook authors to emulate
> what "git add" would do to these arguments and options, and they
> will certainly get it wrong.
> 
> I wonder if we can split write_locked_index() into two so that
> writing out the in-core index to the temporary/lockfile can happen
> separately from the call to commit_locked_index().  If we can do so,
> then the following would become a viable and better implementation
> of this new feature to run the "pre-add" hook:
> 
>  * Determine if we will need to run this "pre-add" hook, at the
>    location in the code you addded the run_hooks_opt() invocation,
>    but do *NOT* run any hook there yet.
> 
>  * Instead, create a temporary copy of the index file if the above
>    says "Yes, we are going to run the hook".
> 
>  * Let the code path to update the in-core index, i.e., letting
>    everythning up to the "finish:" label to run normally.
> 
>  * Perform the first-half of the write_locked_index(), writing the
>    new index contents into the lockfile, but stopping before
>    committing it to the final name.
> 
>  * If we are running the hook, run it with two arguments, the name
>    of the temporary copy of the original index we created earlier,
>    and the name of this lockfile that has the proposed contents of
>    the index if the hook allowed "git add" to proceed.
> 
>  * If we ran the hook and hook succeeded, or if we did not have to
>    run the hook at all, then commit the lockfile.  Otherwise abort
>    the "git add" command and rollback_lock_file().
> 
>  * Remove the temporary file we created earlier (if any).
> 
> Your hooks can "GIT_INDEX_FILE=$1 git diff --cached --name-only" to
> find out which paths already had changes added before this
> invocation of "git add", and similarly using $2 get the list of
> paths that will add further changes with this invocation.  The
> latter set of paths you can inspect to see if you like the
> additional changes brought in, perhaps like
> 
>     #!/bin/sh
>     paths=$(GIT_INDEX_FILE=$2 git diff --cached --name-only)
>     GIT_INDEX_FILE=$1 git diff $paths >patch.txt
> 
>     if grep "^+.*secret" patch.txt
>     then
>         echo "do not divulge company secret!" >&2
> 	exit 1
>     fi
> 
> or something.
> 
>

"Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 6192daeb03..c864ce272d 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -10,7 +10,7 @@ SYNOPSIS
>  [synopsis]
>  git add [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
>  	[--edit | -e] [--[no-]all | -A | --[no-]ignore-removal | [--update | -u]] [--sparse]
> -	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
> +	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] [--no-verify]
>  	[--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
>  	[--] [<pathspec>...]
>  
> @@ -42,6 +42,10 @@ use the `--force` option to add ignored files. If you specify the exact
>  filename of an ignored file, `git add` will fail with a list of ignored
>  files. Otherwise it will silently ignore the file.
>  
> +A pre-add hook can be run to inspect or reject the proposed index update
> +after `git add` computes staging and writes it to the index lockfile,
> +but before writing it to the final index. See linkgit:githooks[5].
>
> +`--no-verify`::
> +	Bypass the pre-add hook if it exists. See linkgit:githooks[5] for
> +	more information about hooks.

I'll leave it up to others to comment on and make concrete
suggestions for the formatting and markups, but the word pre-add the
users must use verbatim that is not marked up in any way would not
look good in the documentation.

Is it and will it always be only the pre-add hook that this option
will bypass, or if we ever add another hook that decides to interfere,
will that hook also be turned off with this option?  This reads like
the former, but the intent would be the latter, no?

I'll also leve it up to others (including the original author of the
patch) to propose a better wording here, as I am not good at naming
things ;-)


> +pre-add
> +~~~~~~~
> +
> +This hook is invoked by linkgit:git-add[1], and can be bypassed with the
> +`--no-verify` option. It is not invoked for `--interactive`, `--patch`,
> +`--edit`, or `--dry-run`.
> +
> +It takes two parameters: the path to a copy of the index before this
> +invocation of `git add`, and the path to the lockfile containing the
> +proposed index after staging. It does not read from standard input.
> +If no index exists yet, the first parameter names a path that does not
> +exist and should be treated as an empty index. No special environment
> +variables are set. The hook is invoked after the index has been updated

What are "special environment variables"?  What happens, for
example, if the end user has an "special environment variable" set
and exported when running "git add"---are you unexporting them?
E.g., Does GIT_INDEX_FILE environment variable visible to the hook
when you do this ...

    $ GIT_INDEX_FILE=.git/alt-index git add .

... and if so, what value does it have?

In other words, is it worth spelling this "special environment
variables" thing out?

> +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> +		int fd_in, status;
> +		const char *index_file = repo_get_index_file(repo);
> +		char *template;
> +
> +		run_pre_add = 1;
> +		template = xstrfmt("%s.pre-add.XXXXXX", index_file);
> +		orig_index = xmks_tempfile(template);
> +		free(template);
> +
> +		fd_in = open(index_file, O_RDONLY);
> +		if (fd_in >= 0) {
> +			status = copy_fd(fd_in, get_tempfile_fd(orig_index));
> +			if (close(fd_in))
> +				die_errno(_("unable to close index for pre-add hook"));
> +			if (close_tempfile_gently(orig_index))
> +				die_errno(_("unable to close temporary index copy"));
> +			if (status < 0)
> +				die(_("failed to copy index for pre-add hook"));
> +		} else if (errno == ENOENT) {
> +			orig_index_path = xstrdup(get_tempfile_path(orig_index));
> +			if (delete_tempfile(&orig_index))
> +				die_errno(_("unable to remove temporary index copy"));
> +		} else {
> +			die_errno(_("unable to open index for pre-add hook"));
> +		}
> +	}

Do we really need to create a copy of the file?  I am just asking
without knowing the answer myself, but given that the general
architecture of file writing used in our codebase, which is to (1)
prepare a new temporary file, (2) write new contents to that
temporary file, and then finally (3) rename the temporary file to
the final location, I would expect that between the time the control
passes this point and the latter half of write_locked_index() calls
commit_locked_index(), the original index file would not be touched
by anybody, and can be readable by the hook.

> +	if (run_pre_add && !exit_status && repo->index->cache_changed) {
> +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +
> +		if (write_locked_index(repo->index, &lock_file, 0))
> +			die(_("unable to write new index file"));

This mimics the pattern used in builtin/commit.c:prepare_index()
that populates the index file (the real one, when making a
non-partial commit, or the temporary one when making a partial
commit), closes it, and let us later commit or roll back depending
on what happens in between.  Looks sensible (but I have to admit
that I may have missed resource leakage etc., as I didn't seriously
look for such flaws).

Shouldn't the die() message mirror the wording used there, i.e.,
"unable to create temporary index" or something, or is this fine, as
it will become the new index file once the hook approves?  I dunno.

Thanks.

> +		strvec_push(&opt.args, orig_index ? get_tempfile_path(orig_index) :
> +					     orig_index_path);
> +		strvec_push(&opt.args, get_lock_file_path(&lock_file));
> +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> +			rollback_lock_file(&lock_file); /* hook rejected */
> +			exit_status = 1;
> +		} else {
> +			if (commit_lock_file(&lock_file)) /* hook approved */
> +				die(_("unable to write new index file"));
> +		}
> +	} else {
> +		if (write_locked_index(repo->index, &lock_file,
> +				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
> +			die(_("unable to write new index file"));
> +	}

> the word pre-add ... would not look good

Originally, I wanted to call these pre-staging hooks. The ugly pre-add wording was an artifact of my attempt to narrow the scope. The goal here was to be as conservative as possible because I thought this concept would be more controversial. This implementation didn't contain hooks for stash/merge/rebase/cherry-pick, which modify the index in their own ways. It wasn't a hook for `commit -a` nor reset/checkout/restore either. I felt it excessively ambitious to name this the pre-staging hook, especially as my first contribution.

Ideally however, I think there should be a category of hooks called pre-staging hooks, with this as the flagship one, and it would make sense, for both aesthetic and future-proofing reasons, for the githooks docs to use that phrasing. 

> Is it and will it always be only the pre-add hook that this option
> will bypass, or if we ever add another hook that decides to interfere,
> will that hook also be turned off with this option?  This reads like
> the former, but the intent would be the latter, no?

As it stands, the no-verify flag is only used in the guard for the "pre-add" hook given the limited scope I aimed for. The implementation could be futureproofed in a way where a string could be passed to the --no-verify flag, each with a unique boolean to guard different hooks. If the flag is set but no strings are passed, then we can assume the user wants no hooks to run, and all of them can be disabled. I thought it overengineering to add something like that in the initial commit, but am open to doing so.

> What is a special environment variable?

That's hilarious. I suppose there's nothing "special" about them, I only meant to say that no unexpected environment variables were being set or unset by the implementation. It was mostly to distinguish this from the original implementation that passed GIT_INDEX_FILE as an env-var which you correctly noted didn't even make sense. In hindsight, I don't see much value in spelling this out unless anyone thinks it would help users distinguish from pre-commit hooks in some useful way.

> Do we really need to create a copy of this [index] file? 

Lockfile protocol should prevent index from being modified. It probably could be as easy as 1) write proposed index -> index.lock and run the hook with $1=index $2=index.lock. Good point. I'll try this out and push it if it works.

> Shouldn't the die() message mirror the wording used there, i.e.,
> "unable to create temporary index" or something, or is this fine, as
> it will become the new index file once the hook approves? 

Answer depends on how the rewrite without index-copying goes. I'll be more conscientious of die messaging in the next commit.

In all, I'd like hooks for pre-staging to be the operative concept here, not pre-add, for more reasons than just the word's poor aesthetics. With interest/approval, I can change the --no-verify implementation to be more generic, although I'm not sure if it's worth actually adding any other pre-staging hooks yet because I haven't seen anyone ask for anything besides gates before add. 

Thanks again

Chandra Kethi-Reddy

Sent with Proton Mail secure email.

On Thursday, February 12th, 2026 at 1:20 AM, Junio C Hamano <gitster@pobox.com> wrote:

> "Chandra Kethi-Reddy via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> > index 6192daeb03..c864ce272d 100644
> > --- a/Documentation/git-add.adoc
> > +++ b/Documentation/git-add.adoc
> > @@ -10,7 +10,7 @@ SYNOPSIS
> >  [synopsis]
> >  git add [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | -i] [--patch | -p]
> >  	[--edit | -e] [--[no-]all | -A | --[no-]ignore-removal | [--update | -u]] [--sparse]
> > -	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize]
> > +	[--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing] [--renormalize] [--no-verify]
> >  	[--chmod=(+|-)x] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> >  	[--] [<pathspec>...]
> >
> > @@ -42,6 +42,10 @@ use the `--force` option to add ignored files. If you specify the exact
> >  filename of an ignored file, `git add` will fail with a list of ignored
> >  files. Otherwise it will silently ignore the file.
> >
> > +A pre-add hook can be run to inspect or reject the proposed index update
> > +after `git add` computes staging and writes it to the index lockfile,
> > +but before writing it to the final index. See linkgit:githooks[5].
> >
> > +`--no-verify`::
> > +	Bypass the pre-add hook if it exists. See linkgit:githooks[5] for
> > +	more information about hooks.
> 
> I'll leave it up to others to comment on and make concrete
> suggestions for the formatting and markups, but the word pre-add the
> users must use verbatim that is not marked up in any way would not
> look good in the documentation.
> 
> Is it and will it always be only the pre-add hook that this option
> will bypass, or if we ever add another hook that decides to interfere,
> will that hook also be turned off with this option?  This reads like
> the former, but the intent would be the latter, no?
> 
> I'll also leve it up to others (including the original author of the
> patch) to propose a better wording here, as I am not good at naming
> things ;-)
> 
> 
> > +pre-add
> > +~~~~~~~
> > +
> > +This hook is invoked by linkgit:git-add[1], and can be bypassed with the
> > +`--no-verify` option. It is not invoked for `--interactive`, `--patch`,
> > +`--edit`, or `--dry-run`.
> > +
> > +It takes two parameters: the path to a copy of the index before this
> > +invocation of `git add`, and the path to the lockfile containing the
> > +proposed index after staging. It does not read from standard input.
> > +If no index exists yet, the first parameter names a path that does not
> > +exist and should be treated as an empty index. No special environment
> > +variables are set. The hook is invoked after the index has been updated
> 
> What are "special environment variables"?  What happens, for
> example, if the end user has an "special environment variable" set
> and exported when running "git add"---are you unexporting them?
> E.g., Does GIT_INDEX_FILE environment variable visible to the hook
> when you do this ...
> 
>     $ GIT_INDEX_FILE=.git/alt-index git add .
> 
> ... and if so, what value does it have?
> 
> In other words, is it worth spelling this "special environment
> variables" thing out?
> 
> > +	if (!show_only && !no_verify && find_hook(repo, "pre-add")) {
> > +		int fd_in, status;
> > +		const char *index_file = repo_get_index_file(repo);
> > +		char *template;
> > +
> > +		run_pre_add = 1;
> > +		template = xstrfmt("%s.pre-add.XXXXXX", index_file);
> > +		orig_index = xmks_tempfile(template);
> > +		free(template);
> > +
> > +		fd_in = open(index_file, O_RDONLY);
> > +		if (fd_in >= 0) {
> > +			status = copy_fd(fd_in, get_tempfile_fd(orig_index));
> > +			if (close(fd_in))
> > +				die_errno(_("unable to close index for pre-add hook"));
> > +			if (close_tempfile_gently(orig_index))
> > +				die_errno(_("unable to close temporary index copy"));
> > +			if (status < 0)
> > +				die(_("failed to copy index for pre-add hook"));
> > +		} else if (errno == ENOENT) {
> > +			orig_index_path = xstrdup(get_tempfile_path(orig_index));
> > +			if (delete_tempfile(&orig_index))
> > +				die_errno(_("unable to remove temporary index copy"));
> > +		} else {
> > +			die_errno(_("unable to open index for pre-add hook"));
> > +		}
> > +	}
> 
> Do we really need to create a copy of the file?  I am just asking
> without knowing the answer myself, but given that the general
> architecture of file writing used in our codebase, which is to (1)
> prepare a new temporary file, (2) write new contents to that
> temporary file, and then finally (3) rename the temporary file to
> the final location, I would expect that between the time the control
> passes this point and the latter half of write_locked_index() calls
> commit_locked_index(), the original index file would not be touched
> by anybody, and can be readable by the hook.
> 
> > +	if (run_pre_add && !exit_status && repo->index->cache_changed) {
> > +		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> > +
> > +		if (write_locked_index(repo->index, &lock_file, 0))
> > +			die(_("unable to write new index file"));
> 
> This mimics the pattern used in builtin/commit.c:prepare_index()
> that populates the index file (the real one, when making a
> non-partial commit, or the temporary one when making a partial
> commit), closes it, and let us later commit or roll back depending
> on what happens in between.  Looks sensible (but I have to admit
> that I may have missed resource leakage etc., as I didn't seriously
> look for such flaws).
> 
> Shouldn't the die() message mirror the wording used there, i.e.,
> "unable to create temporary index" or something, or is this fine, as
> it will become the new index file once the hook approves?  I dunno.
> 
> Thanks.
> 
> > +		strvec_push(&opt.args, orig_index ? get_tempfile_path(orig_index) :
> > +					     orig_index_path);
> > +		strvec_push(&opt.args, get_lock_file_path(&lock_file));
> > +		if (run_hooks_opt(repo, "pre-add", &opt)) {
> > +			rollback_lock_file(&lock_file); /* hook rejected */
> > +			exit_status = 1;
> > +		} else {
> > +			if (commit_lock_file(&lock_file)) /* hook approved */
> > +				die(_("unable to write new index file"));
> > +		}
> > +	} else {
> > +		if (write_locked_index(repo->index, &lock_file,
> > +				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > +			die(_("unable to write new index file"));
> > +	}
> 
>

Chandra <Chandrakr@pm.me> writes:

>> the word pre-add ... would not look good
>
> Originally, I wanted to call these pre-staging hooks.

I was not talking about the choice of words.  If pre-commit
interferes before a commit is made in 'git commit', pre-add is a
natural phrase to use to interfere 'git add'.

It was a comment only on how it is typeset in the documentation,
e.g., should it be `pre-add` (for verbatim), 'pre-add', _pre_add_,
etc.

In the git-commit documentation, pre-commit is always verbatim. For consistency, pre-add typeset as verbatim makes sense.


Chandra Kethi-Reddy

Sent from Proton Mail for iOS.

-------- Original Message --------
On Thursday, 02/12/26 at 02:56 Junio C Hamano <gitster@pobox.com> wrote:
Chandra <Chandrakr@pm.me> writes:

>> the word pre-add ... would not look good
>
> Originally, I wanted to call these pre-staging hooks.

I was not talking about the choice of words.  If pre-commit
interferes before a commit is made in 'git commit', pre-add is a
natural phrase to use to interfere 'git add'.

It was a comment only on how it is typeset in the documentation,
e.g., should it be `pre-add` (for verbatim), 'pre-add', _pre_add_,
etc.